Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Merged by Bors] - feat: add support to string interpolation #3138

Closed

Conversation

morenol
Copy link
Contributor

@morenol morenol commented Apr 14, 2023

Solves #3127

With this change we could use something like:

http:
  headers:
   - "Authorization: JWT ${{ secrets.JWT_TOKEN }}"

And that should be resolved as

  • "Authorization: JWT MYSECRET_TOKEN"

@morenol morenol force-pushed the lmm/support-string-interpolation branch from aeab496 to f3a6d64 Compare April 14, 2023 16:07
Cargo.toml Outdated Show resolved Hide resolved
@morenol morenol marked this pull request as ready for review April 14, 2023 16:40
@morenol morenol requested a review from galibey April 14, 2023 16:50
@galibey
Copy link
Contributor

galibey commented Apr 14, 2023

This implementation only covers string interpolation for SecretString type, right? But we need it for other types as well. At minimum other String as well.

We need to come up with a such abstraction that it will be easier later to add other sources of data, not only secrets but envs, for example.

@morenol
Copy link
Contributor Author

morenol commented Apr 14, 2023

This implementation only covers string interpolation for SecretString type, right?

Yes, only for SecretString

@morenol
Copy link
Contributor Author

morenol commented Apr 14, 2023

@galibey regarding the String case, I added a new method to the SecretStore trait. The method is resolve_str where you can pass any String and it will resolve it.

@morenol
Copy link
Contributor Author

morenol commented Apr 14, 2023

We need to come up with a such abstraction that it will be easier later to add other sources of data, not only secrets but envs, for example.

Maybe creating a new Wrapper type for String, were we could call SecretStore::resolve, EnvStore::resolve, and any other resolvers as needed?

@galibey
Copy link
Contributor

galibey commented Apr 14, 2023

@galibey regarding the String case, I added a new method to the SecretStore trait. The method is resolve_str where you can pass any String and it will resolve it.

I meant it works with yaml string only if its Rust counterpart is SecretString, not String.
And other yaml data types is not covered, but according to the linked issue, they should.

Upd. My bad, only string type is required

@morenol
Copy link
Contributor Author

morenol commented Apr 14, 2023

I meant it works with yaml string only if its Rust counterpart is SecretString, not String. And other yaml data types is not covered, but according to the linked issue, they should.

The find_secrets function will work as expected (We will be able to detect which are the secret names), but yes, we don't automatically resolve them on connector config. Do you think that we should do an automatic resolve() for every string in the input?

@galibey
Copy link
Contributor

galibey commented Apr 14, 2023

Maybe creating a new Wrapper type for String, were we could call SecretStore::resolve, EnvStore::resolve, and any other resolvers as needed?

Yes, this is a possible approach. The other one could be is to parse the config data into string, replace variables and then parse the resulting string into config type.

@galibey
Copy link
Contributor

galibey commented Apr 14, 2023

The find_secrets function will work as expected (We will be able to detect which are the secret names), but yes, we don't automatically resolve them on connector config. Do you think that we should do an automatic resolve() for every string in the input?

Could be so, yes.

@morenol
Copy link
Contributor Author

morenol commented Apr 14, 2023

Maybe creating a new Wrapper type for String, were we could call SecretStore::resolve, EnvStore::resolve, and any other resolvers as needed?

Yes, this is a possible approach. The other one could be is to parse the config data into string, replace variables and then parse the resulting string into config type.

Implemented that behavior in f6331950a1a834b32bb9bb9db0e3425c55b99a29

Copy link
Contributor

@galibey galibey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!. Just a minor comment.

crates/fluvio-connector-package/src/secret.rs Outdated Show resolved Hide resolved

static SECRET_STORE: OnceCell<Box<dyn SecretStore>> = OnceCell::new();

lazy_static! {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

avoid lazy_static. should use once_cell

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

@morenol morenol requested a review from sehz April 18, 2023 16:32
@morenol morenol requested a review from sehz April 18, 2023 17:34
@morenol morenol force-pushed the lmm/support-string-interpolation branch from cda51fa to 1718b20 Compare April 18, 2023 19:07
@morenol
Copy link
Contributor Author

morenol commented Apr 18, 2023

@sehz could you take another look?

crates/fluvio-connector-package/src/secret.rs Outdated Show resolved Hide resolved
crates/fluvio-connector-package/src/secret.rs Outdated Show resolved Hide resolved
@morenol morenol marked this pull request as draft April 18, 2023 23:18
@morenol morenol force-pushed the lmm/support-string-interpolation branch 3 times, most recently from 19de5d2 to 33f6800 Compare April 20, 2023 14:41
@morenol morenol force-pushed the lmm/support-string-interpolation branch from 2fced09 to 001f5c0 Compare April 20, 2023 22:38
@morenol
Copy link
Contributor Author

morenol commented Apr 20, 2023

@sehz please take another look

@morenol morenol force-pushed the lmm/support-string-interpolation branch 2 times, most recently from 196676c to f719b90 Compare April 25, 2023 18:12
@morenol morenol marked this pull request as draft April 25, 2023 18:13
@morenol morenol force-pushed the lmm/support-string-interpolation branch 6 times, most recently from 8df28cc to e6f9077 Compare April 26, 2023 14:51
@morenol morenol force-pushed the lmm/support-string-interpolation branch from e6f9077 to 1d0435a Compare April 26, 2023 15:41
@morenol morenol marked this pull request as ready for review April 26, 2023 15:50
@morenol morenol requested a review from sehz April 26, 2023 15:50
Copy link
Contributor

@sehz sehz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good to me. One minor ask

@morenol morenol requested a review from sehz April 26, 2023 17:18
Copy link
Contributor

@sehz sehz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@morenol
Copy link
Contributor Author

morenol commented Apr 26, 2023

bors r+

bors bot pushed a commit that referenced this pull request Apr 26, 2023
Solves #3127 

With this change we could use something like:

```
http:
  headers:
   - "Authorization: JWT ${{ secrets.JWT_TOKEN }}"
```

And that should be resolved as

   - "Authorization: JWT MYSECRET_TOKEN"
@bors
Copy link

bors bot commented Apr 26, 2023

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat: add support to string interpolation [Merged by Bors] - feat: add support to string interpolation Apr 26, 2023
@bors bors bot closed this Apr 26, 2023
@morenol morenol deleted the lmm/support-string-interpolation branch May 8, 2023 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants